Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ls] implement multiple workspace folders in MonacoWorkspace #7182

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

JanKoehnlein
Copy link
Contributor

Fixes #6108

Signed-off-by: Jan Koehnlein jan.koehnlein@typefox.io

What it does

This PR adds support of multiple workspace root folders to Monaco workspace.
Language servers will be notified with a didChangeWorkspaceFolders notification whenever the user adds or removes a workspace folder.

How to test

Add/remove workspace folders in Theia and watch extensions with language servers get notified.

Reminder for reviewers

@vince-fugnitto vince-fugnitto added monaco issues related to monaco multi-root issues related to multi-root support labels Feb 19, 2020
@kittaakos
Copy link
Contributor

I do not understand this explicit vscode dependency. I can see monaco-languageclient also imports them.

@kittaakos
Copy link
Contributor

I do not understand this explicit vscode dependency.

I see. They're from here: https://github.com/microsoft/vscode/blob/master/src/vs/vscode.d.ts#L8432-L8452.

@JanKoehnlein
Copy link
Contributor Author

Sorry for the sloppy initial commit. I've changed the ready resolution and added the missing dev dependency on @types/vscode.

@JanKoehnlein
Copy link
Contributor Author

I addressed your comments and re-implemented the workspaceFolder init/updated

@akosyakov
Copy link
Member

@JanKoehnlein the build is failing, for some reasons it cannot find monaco namespace anymore

@JanKoehnlein
Copy link
Contributor Author

I had this sporadically here, too. I guess we should not add a dependency to @types/vscode but define the WorkspaceFolder and WorkspaceFolderChangeEvent interfaces ourselves

@kittaakos
Copy link
Contributor

Thanks for the updates, @JanKoehnlein. Once the build is green I am going to verify it with one or two VS Code LS extensions.

@kittaakos
Copy link
Contributor

It throws an error at startup:

monaco-workspace.ts:214 Uncaught (in promise) TypeError: Cannot read property 'map' of undefined
    at MonacoWorkspace.push.../../packages/monaco/lib/browser/monaco-workspace.js.MonacoWorkspace.updateWorkspaceFolders (monaco-workspace.ts:214)
    at MonacoWorkspace.<anonymous> (monaco-workspace.ts:200)
    at step (monaco-workspace.ts:15)
    at Object.next (monaco-workspace.ts:15)
    at fulfilled (monaco-workspace.ts:15)

You get the value before you set it:
Get:

const oldWorkspaceUris = this.workspaceFolders.map(folder => folder.uri.toString());

Set:
this._workspaceFolders = newWorkspaceUris.map(this.toWorkspaceFolder);

Fixes #6108

Signed-off-by: Jan Koehnlein <jan.koehnlein@typefox.io>
@JanKoehnlein
Copy link
Contributor Author

JanKoehnlein commented Feb 20, 2020

Wow. Looks like I should get more sleep. Initialization of the property with an empty array was missing.

@kittaakos
Copy link
Contributor

From the LSP specs:

The workspace/workspaceFolders request is sent from the server to the client to fetch the current open list of workspace folders. Returns null in the response if only a single file is open in the tool. Returns an empty array if a workspace is open but no folders are configured.

If I open Theia without a workspace and then open a single file (e.g.: main.go), the monaco-workspace returns with an empty array instead of null. Note, the LS works.

@akosyakov
Copy link
Member

akosyakov commented Feb 21, 2020

@kittaakos MonacoWorkspace confirms to VS Code API, not to LSP, transformation from VS Code api to LSP happens here: https://github.com/microsoft/vscode-languageserver-node/blob/50ffd38681b3fc8dde75a70accfa3c29db1889f3/client/src/workspaceFolders.ts#L45-L53

Docs for VS Code APIs: https://github.com/microsoft/vscode/blob/c975df3b59ac221f70d525498ae6ff81bce77a10/src/vs/vscode.d.ts#L8515-L8519

Although It says the same 😄 WorkspaceService has opened getter to check whether a folder is opened.

@kittaakos kittaakos self-requested a review February 21, 2020 08:59
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried it with the TS LS, and the vscode-go extension in the browser. I added then removed a few workspaces, even ones with whitespace in their names. It worked as expected: I had diagnostics and content assist support in my newly added workspaces. Thank you for the path patch 👍

@JanKoehnlein JanKoehnlein merged commit 6a6a673 into master Feb 21, 2020
@JanKoehnlein
Copy link
Contributor Author

Thanks a lot for the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monaco issues related to monaco multi-root issues related to multi-root support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[multi-workspace][ls] Support workspace/didChangeWorkspaceFolders
4 participants